Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding unit and name attributes to layers #36

Merged
merged 5 commits into from
Jul 14, 2024
Merged

adding unit and name attributes to layers #36

merged 5 commits into from
Jul 14, 2024

Conversation

prayner
Copy link
Contributor

@prayner prayner commented Jul 9, 2024

addresses #16


Description

Checklist

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)

Notes

@prayner prayner requested a review from a team July 9, 2024 05:43
Copy link
Collaborator

@lewisjared lewisjared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add the required attributes to the total as well as the layers in the sum_layers function.

We should also add a test to the test_om_prior.py which verifies that the units are in the output. This helps to ensure that they don't get lost if we do further changes. This one is a bit annoying because it requires running the whole workflow to test this so it can be a bit slow.

A simple test like this could be sufficient for now:

def test_required_attributes(output_domain_xr):
    assert output_domain_xr.variables["OCH4_TOTAL"].attrs == {
        "units": "kg/m^2/s",
        "long_name": "OCH4_TOTAL",
    }
    assert output_domain_xr.variables["OCH4_WETLANDS"].attrs == {
        "units": "kg/m^2/s",
        "long_name": "OCH4_WETLANDS",
    }

This currently fails until the fix to the total has been implemented.

We should have a session on how to run pytest and the test suite.

src/openmethane_prior/omOutputs.py Outdated Show resolved Hide resolved
src/openmethane_prior/omOutputs.py Outdated Show resolved Hide resolved
@prayner
Copy link
Contributor Author

prayner commented Jul 10, 2024 via email

@lewisjared
Copy link
Collaborator

@prayner Are you able to see any line-specific comments or are they not available in emacs?

@prayner
Copy link
Contributor Author

prayner commented Jul 10, 2024 via email

@prayner
Copy link
Contributor Author

prayner commented Jul 10, 2024 via email

Copy link
Collaborator

@lewisjared lewisjared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok good to know.

For constants, "SCREAMING_SNAKE_CASE" is the preference. If we don't change this as part of this PR it will likely later be cleaned up.

I had another comment to use setncatts to set all the values at the same time instead of looping.

I'll fix them up with the tests

@lewisjared
Copy link
Collaborator

@prayner Can I add the tests to this branch? If so please assign this PR to me

* main: (41 commits)
  chore: Copy .env file
  chore: Include dev and tests
  chore: Include dev and tests
  chore: Include dev and tests
  refactor: Move reproject_raster_inputs to raster
  chore: remove poetry
  chore: Fix image
  chore: vendor the reproject code
  chore: Try avoid blowing up the cache size by only caching the final image
  chore: Simplify
  chore: Try again
  chore: Try again
  chore: Use venv pip
  chore: ls
  chore: Another attempt 5
  chore: Another attempt 4
  chore: Another attempt 3
  chore: Another attempt 2
  chore: Another attempt
  chore: testing
  ...
@lewisjared lewisjared merged commit 0f70d70 into main Jul 14, 2024
7 checks passed
@lewisjared lewisjared deleted the attribute-fix branch July 14, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants